feat: add to the usecase and viewmodel of the get_all_members and get…#238
Conversation
…_all_members_admin routes the logic to return the strikes_id that the user has recieved in the semester
There was a problem hiding this comment.
Pull request overview
This PR extends the get_all_members and get_all_members_admin flows to include the list of strike identifiers (strikes_id) associated with each member for the selected semester window.
Changes:
- Populates
member.strikes_idin bothGetAllMembersUsecaseandGetAllMembersAdminUsecase. - Adds
strikes_idto bothget_all_membersandget_all_members_adminviewmodel outputs. - Updates controller/viewmodel tests to assert the new
strikes_idfield in responses.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/modules/get_all_members/app/get_all_members_usecase.py |
Adds member.strikes_id assignment during member aggregation. |
src/modules/get_all_members/app/get_all_members_viewmodel.py |
Includes strikes_id in serialized member output. |
src/modules/get_all_members_admin/app/get_all_members_admin_usecase.py |
Adds member.strikes_id assignment for admin aggregation. |
src/modules/get_all_members_admin/app/get_all_members_admin_viewmodel.py |
Adds strikes_id field to admin viewmodel and output dict. |
tests/modules/get_all_members/app/test_get_all_members_viewmodel.py |
Updates expected payloads to include strikes_id. |
tests/modules/get_all_members/app/test_get_all_members_controller.py |
Updates expected controller response to include strikes_id. |
tests/modules/get_all_members_admin/app/test_get_all_members_admin_viewmodel.py |
Updates expected admin viewmodel payloads to include strikes_id. |
tests/modules/get_all_members_admin/app/test_get_all_members_admin_controller.py |
Updates expected admin controller response to include strikes_id. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| member.strikes_allowed= 4 | ||
|
|
||
| member.strikes= len(member_list_strike_this_sem) | ||
| member.strikes_id= [s.strike_id for s in member_list_strike_this_sem] | ||
| member.hours_worked = hours_worked.get(member_user_id, 0) if is_admin else None |
There was a problem hiding this comment.
member_list_strike_this_sem is only assigned inside if member_list_strikes:, but it’s used unconditionally to compute member.strikes and member.strikes_id. When get_strike_by_target_id returns None (or an empty list), this will raise UnboundLocalError. Initialize member_list_strikes to ... or [] and always build member_list_strike_this_sem (defaulting to an empty list) before taking len() / mapping ids.
| 'user_id' : self.user_id, | ||
| 'strikes' : self.strikes, | ||
| 'strikes_id' : self.strikes_id, | ||
| 'strikes_allowed' : self.strikes_allowed, |
There was a problem hiding this comment.
Returning strikes_id for every member from the non-admin get_all_members response can leak strike identifiers to regular users. In this codebase, GetStrikeUsecase only checks that the requester is active (no ownership/admin authorization), so exposing IDs here makes it trivial to fetch strike details for other users. Consider omitting strikes_id in this endpoint (or only including it for the requester / admins).
| user_id: str | ||
| hours_worked: int | ||
| strikes: int | ||
| strikes_id: list |
There was a problem hiding this comment.
Type annotation strikes_id: list is too generic and inconsistent with other viewmodels (e.g., GetMemberViewModel uses List[str]). Prefer List[str] (or list[str] if the project targets Python 3.9+) for clearer contracts and better static checking.
| strikes_id: list | |
| strikes_id: List[str] |
…ll_members_admin_viewmodel
…_all_members_admin routes the logic to return the strikes_id that the user has recieved in the semester